[scramjet/controller] polish controller api for consumers#44
[scramjet/controller] polish controller api for consumers#44Percslol wants to merge 7 commits intoHeyPuter:developfrom
Conversation
|
and do we want |
There was a problem hiding this comment.
Pull request overview
This PR aims to polish the @mercuryworkshop/scramjet-controller consumer-facing API and align the demo with the updated controller surface, while also removing the rewriter_demo package and updating lockfiles accordingly.
Changes:
- Update controller API to expose
config/scramjetConfigand move consumer usage towardcontroller.wait()andcontroller.scramjetConfig.flags. - Remove the
packages/scramjet/packages/rewriter_demodemo package and its dependencies from lockfiles. - Refresh pnpm lockfiles (tooling/version shifts, dependency graph changes).
Reviewed changes
Copilot reviewed 9 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updates workspace dependency resolutions; removes rewriter_demo importer and adjusts tooling deps. |
| packages/scramjet/pnpm-lock.yaml | Removes set-cookie-parser entry from scramjet workspace lockfile. |
| packages/scramjet/packages/rewriter_demo/src/style.css | Deleted as part of removing rewriter_demo. |
| packages/scramjet/packages/rewriter_demo/src/main.js | Deleted as part of removing rewriter_demo. |
| packages/scramjet/packages/rewriter_demo/public/vite.svg | Deleted as part of removing rewriter_demo. |
| packages/scramjet/packages/rewriter_demo/package.json | Deleted as part of removing rewriter_demo. |
| packages/scramjet/packages/rewriter_demo/index.html | Deleted as part of removing rewriter_demo. |
| packages/scramjet/packages/rewriter_demo/.gitignore | Deleted as part of removing rewriter_demo. |
| packages/scramjet/packages/demo/src/index.tsx | Updates demo initialization to new controller API and exports controller. |
| packages/scramjet/packages/demo/src/App.tsx | Updates flags editing to write into controller.scramjetConfig.flags. |
| packages/scramjet/packages/controller/src/types.d.ts | Adjusts exported types but currently breaks by removing an import that is still referenced. |
| packages/scramjet/packages/controller/src/inject.ts | Aligns injected runtime types and client init behavior with new controller config structure. |
| packages/scramjet/packages/controller/src/index.ts | Introduces config/scramjetConfig on Controller and adds merge support, plus inject script changes. |
Files not reviewed (2)
- packages/scramjet/pnpm-lock.yaml: Language not supported
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
packages/scramjet/packages/demo/src/index.tsx:57
controlleris declared without an initializer and with a non-optional type, but it’s read inswapTransportbeforeinit()necessarily assigns it. UnderstrictPropertyInitialization/definite assignment analysis this pattern commonly triggers “Variable 'controller' is used before being assigned”, and at runtime it can still beundefinedifswapTransportis called early. Declare it asController | undefined(or use a definite assignment assertionlet controller!: …if you guarantee assignment before use) and keep the guard consistent.
let controller: ScramjetControllerGlobal.Controller;
export async function swapTransport(wispUrl: string) {
const nextTransport = new LibcurlClient({
wisp: wispUrl,
});
transport = nextTransport;
if (!controller) {
return;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| constructor(public init: ControllerInit) { | ||
| this.transport = init.transport; | ||
| this.id = makeId(); | ||
| this.prefix = config.prefix + this.id + "/"; | ||
| this.config = deepMerge(config, init.config); | ||
| this.scramjetConfig = deepMerge($scramjet.defaultConfig, scramjetConfig); | ||
| this.scramjetConfig = deepMerge(this.scramjetConfig, init.scramjetConfig); | ||
| this.prefix = this.config.prefix + this.id + "/"; |
There was a problem hiding this comment.
The new config?: Partial<Config> option is merged into this.config, but several code paths still use the module-level config constant (e.g. loadScramjetWasm() and the virtual wasm request handling). This means per-controller overrides like init.config.wasmPath / virtualWasmPath won’t actually take effect. Use this.config (or pass the resolved config into helpers) consistently anywhere the controller reads paths/codec config.
| function deepMerge(target: any, source: any): any { | ||
| for (const key in source) { | ||
| if (key in target) { | ||
| Object.assign(source[key], deepMerge(target[key], source[key])); | ||
| } | ||
| } | ||
|
|
||
| type ControllerInit = { | ||
| serviceworker: ServiceWorker; | ||
| transport: ProxyTransport; | ||
| }; | ||
| return Object.assign(target || {}, source); | ||
| } |
There was a problem hiding this comment.
deepMerge is not safe for the way it’s used in the constructor: init.config / init.scramjetConfig are optional and can be undefined, but for (const key in source) will throw on undefined/null. It also does Object.assign(source[key], ...) without checking that source[key] is an object, which can throw for primitives, and it mutates both source and target (so calling deepMerge(config, …) can inadvertently mutate the exported config constant and $scramjet.defaultConfig). Consider replacing this with a non-mutating deep merge that treats non-plain objects/arrays as leaf values and gracefully handles undefined sources.
No description provided.